Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a CI build for double precision #669

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

beomki-yeo
Copy link
Contributor

We need to make sure that the traccc is compiled normally for the double precision

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both Release and Debug builds with this setting?

I am absolutely on board with adding this. If hardware resources were not a concern, I could imagine just doubling the number of our tests outright. Adding one more "axis" to our test matrix.

Actually, should we try that? @paulgessinger, I guess you may have the most experience here. With how slow our CI may become if we throw so much work at it. 🤔

@beomki-yeo
Copy link
Contributor Author

Do we need both Release and Debug builds with this setting?

No, running it for Debug only would be ideal for the current setting. I just could not find a elegant way to exclude the Release build

And I think it is going to be too much if we double the amount of tests. I am here assuming that successful tests with the FP32 also guarantee the success with the FP64, hopefully

@beomki-yeo beomki-yeo linked an issue Aug 8, 2024 that may be closed by this pull request
@beomki-yeo beomki-yeo force-pushed the add-double-precision-build branch from 69bcf0a to f0508af Compare August 8, 2024 14:23
@stephenswat
Copy link
Member

I just could not find a elegant way to exclude the Release build

The easy way to do this would be using the matrix.include method, as seen in https://github.com/acts-project/traccc/blob/main/.github/workflows/builds.yml#L60.

In short, you'd add something like this:

        include:
          - platform:
            - name: CUDA
              container: ghcr.io/acts-project/ubuntu2404_cuda:56
              cxx_standard: "20"
              options: -DTRACCC_CUSTOM_SCALARTYPE=double -DDETRAY_CUSTOM_SCALARTYPE=double -DTRACCC_BUILD_CUDA=TRUE -DCMAKE_CUDA_FLAGS="-std=c++20"
              run_tests: false
            build: Release

@paulgessinger
Copy link
Member

@krasznaa Is this on the github or the gitlab side?

@beomki-yeo beomki-yeo force-pushed the add-double-precision-build branch 5 times, most recently from 9545b2d to 588b840 Compare August 9, 2024 01:22
@krasznaa
Copy link
Member

krasznaa commented Aug 9, 2024

Yepp, Pandora's box... 😦

Because of the issues, it could still make sense to set up double versions of pretty much all of the device backends. Otherwise there will always be some code that our tests don't cover. 🤔

It doesn't need to be this PR though. You'll have plenty of work with just making the core (and CUDA) code work correctly like this. We can take care of fixing up everything else in follow-up PRs.

@beomki-yeo beomki-yeo force-pushed the add-double-precision-build branch from 588b840 to 87421d8 Compare August 9, 2024 18:54
@beomki-yeo
Copy link
Contributor Author

While making this work, I also removed the awkward scalar type dependency in the seeding_config and finding_config

@beomki-yeo beomki-yeo requested a review from krasznaa August 14, 2024 05:28
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just a bit uncertain about the simulation changes. Everything else I'm very much on board with. 👍

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's go then! 😄

@beomki-yeo beomki-yeo force-pushed the add-double-precision-build branch 2 times, most recently from b1daa2b to 8388e40 Compare August 15, 2024 14:39
@krasznaa krasznaa force-pushed the add-double-precision-build branch from 8388e40 to 2f3cfd6 Compare August 16, 2024 15:32
@krasznaa krasznaa force-pushed the add-double-precision-build branch from 2f3cfd6 to 41bd4f8 Compare August 17, 2024 08:14
@krasznaa krasznaa merged commit fc69330 into acts-project:main Aug 17, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add double precision build in CI
4 participants